fix(controllers): derive --initial-cluster-state from phase, not from the seed - #355

Second layer of a stack. Depends on #354, which makes the seed eligible for
crash-loop self-heal; this one makes the seed fail loudly in the case that
self-heal cannot otherwise see.

The bug

buildPod rendered etcd's --initial-cluster-state straight off spec.bootstrap:

clusterState := "new"
if !member.Spec.Bootstrap { clusterState = "existing" }

spec.bootstrap is set once at seed creation and never cleared, so the seed's
Pod was built with =new for the entire life of the cluster, not just for
its first boot. Its --initial-cluster is likewise frozen at the bootstrap
value, which names only itself.

Why that is usually invisible, and occasionally very bad

etcd honours --initial-cluster-state only when the data dir is empty. So it is
inert on ordinary restarts, and inert on a corrupt data dir — etcd fails to
boot either way, the Pod crash-loops, and self-heal replaces the member (that is
the path #336 added and #354 extends to the seed).

It is not inert when the seed's data dir comes back empty with the PVC
binding intact: a re-provisioned volume, a PV restored blank, node-local storage
lost on a node reimage. There the two cases diverge sharply:

• a non-seed member gets =existing against a stale --initial-cluster, fails
  loudly (member count is unequal), crash-loops, and is self-healed;
• the seed got =new against an --initial-cluster naming only itself. That is
  a complete, internally consistent bootstrap instruction, so etcd does not
  error. It forms a fresh one-member cluster on the empty dir and reports
  healthy.

The second outcome is worse than the crash-loop it resembles, because it is
quiet. Two things are certain here, and one is not — worth separating.

Certain: the member goes Ready on an empty data dir. Its readiness probe is
/health, which a healthy one-member cluster answers 200. <cluster>-client
selects every member Pod with no role filter, so it immediately takes a share of
client traffic — serving reads from an empty keyspace and accepting writes the
rest of the cluster knows nothing about. Neither self-heal trigger sees any of
it: the Status.PodUID check needs a lost Pod, etcdContainerStuck needs a
not-ready container.

Certain: etcd's own guard against strangers does not fire.
EtcdServer.Process rejects a raft message whose m.To is not the local member
ID (cannot process message to mismatch member) — the check that would normally
fence off a member from a different cluster. Member IDs are derived
deterministically (computeMemberID = sha1 over sorted peer URLs + cluster
token; NewClusterFromURLsMap passes a nil timestamp, so bootstrap IDs carry no
entropy), and nothing about this member changed, so its ID is identical to the
one the survivors still have on file. Their messages address it correctly and go
straight into raft. Same story one level up for the cluster ID and the peer
transport's X-Etcd-Cluster-ID check. The collision is precisely what removes
the loud failure.

Not established: how long the divergence lasts. Once those messages reach
raft, the surviving leader's higher term and mismatched log should pull the
member back into line, most likely via InstallSnapshot. If so the window is
short. That is expected behaviour rather than something observed here, and it is
deliberately not what this PR rests on — a brief window is still wrong reads
served to clients, and a write acknowledged in that window is silently discarded
when the snapshot lands, which is an acknowledged-write loss rather than a stale
read.

The case for =existing does not depend on the divergence being durable. It
rests on not issuing a bootstrap instruction to a member that is not
bootstrapping: =existing fails immediately and deterministically into a
designed, tested recovery path, instead of relying on raft to repair a state the
operator should never have created.

One likely objection: the operator already randomises the cluster token, so
shouldn't the IDs differ? deriveClusterToken (helpers.go:521) is
<namespace>-<cluster>-<uid>, and its entropy is the EtcdCluster's UID — so it
varies across incarnations, which is what stops a stale PVC from a previous
same-named cluster rejoining. A member rebooting inside one incarnation has the
same object, hence the same UID, hence the same token; spec.clusterToken is
copied from status.clusterToken at creation and never rewritten afterwards (no
assignment to it exists outside the two creation-time struct literals). The token
defends the boundary between clusters, not between boots.

The fix

Ask a phase question instead of an identity question. Emit =new only while
nothing yet says this cluster exists — two independent signals, both of which
must agree:

clusterState := "existing"
if member.Spec.Bootstrap && member.Status.MemberID == "" && !clusterFormed {
    clusterState = "new"
}

clusterFormed is cluster.Status.ClusterID != "", read in ensurePod via the
existing clusterFor helper and passed in, which keeps buildPod a pure
function of its arguments.

Requiring both is strictly safer than either alone and cannot misfire:
either signal being set proves the cluster formed, which makes an empty data dir
data loss rather than a pending bootstrap — and =existing failing loudly is
precisely the outcome we want, because #354 then replaces the member.

spec.bootstrap keeps its identity role. It still anchors seed discovery before
clusterID is latched, and it remains a permanent record of which member
bootstrapped; it simply no longer steers ongoing behaviour. Clearing the field
was the obvious alternative and was rejected: it records phase in spec (where
status.clusterID already answers the question with no write at all), it is
correct only if the clearing write is ordered right — a seed cleared before its
Pod ever starts would boot =existing against a self-only --initial-cluster
and never form the cluster — and it destroys the only record of which member was
the seed, which still explains that member's otherwise-anomalous
--initial-cluster.

Why it is safe to roll out

• No spec writes, no Pod restarts. ensurePod never re-templates an
  existing Pod, so running seeds keep their current argv and converge to
  =existing whenever their Pod is next recreated. Nothing moves on upgrade.
• Restore is unaffected. internal/agent/restore.go:140 calls
  snapshot.Restore with InitialCluster/InitialClusterToken, producing a
  complete data dir with a WAL — etcd ignores --initial-cluster-state entirely
  on that path.
• Pause/resume improves. A dormant seed's Pod is recreated on resume; today
  it returns with =new, so a PVC that lost data during the pause would
  silently re-bootstrap. =existing makes that loud.

Residual, deliberately not closed: a seed that formed the cluster while
both memberID and clusterID are still unset — a seconds-wide window — and
loses its data dir inside it would still re-bootstrap. That is today's behaviour
rather than a regression; closing it fully needs a persisted "has booted once"
marker, which is not worth the field.

Tests

TestBuildPod_InitialClusterState — table over the six meaningful states.
Note that nothing asserted this flag before, so this closes a live coverage
gap rather than adjusting existing expectations. The three seed-with-a-formed-
cluster rows fail against the old derivation.

TestEnsurePod_FormedClusterGivesSeedExistingState — the integration half:
proves ensurePod actually reads the parent cluster's clusterID rather than
just accepting a bool. Also fails against the old derivation.

TestSeedDataDirLossDoesNotRebootstrap (e2e, new file) — wipes rather than
corrupts the seed's data dir, because empty-vs-corrupt is exactly where the two
paths diverge and corruption only reaches the already-covered crash-loop. It
writes a sentinel key through the client Service first, then asserts that the
wiped seed is replaced and that the sentinel survives on every ready
member afterwards — checking all of them, so a single member answering out of an
empty store is caught rather than averaged away by whichever endpoint the Service
happens to pick.

Those two are the load-bearing assertions, and both are deterministic: a seed
that re-bootstrapped stays Ready and is therefore never replaced, so the wait
times out regardless of what raft does afterwards. The test additionally probes
the seed for the sentinel while waiting; catching it serving without the sentinel
fast-fails with the precise diagnosis instead of burning the full 15-minute
budget. That probe is opportunistic — missing it proves nothing, since the
surviving leader may already have snapshotted the member back into consistency —
but it cannot produce a false failure, as it fires only on a successful read
returning something other than the value we wrote.

The rest of the diff is mechanical: buildPod gained a parameter, so its ~24
existing test call sites pass false. Two namespace-bound e2e helpers were
generalised (readyMembersIsIn, seedMemberIn) so the new suite can use its own
namespace; the existing wrappers are unchanged for their callers.

Docs

concepts.md replaces the "Known gap: a seed that re-bootstraps" section added
in #354 with "Bootstrap state is a phase, not a member", documenting the derivation
and keeping the failure mode on record as the reason for it.

Verification

go build ./..., go vet -tags e2e ./test/e2e/ and go test ./controllers/
are green. Both new unit tests were confirmed to fail against the previous
derivation before the fix was applied.

The e2e has now run on CI's kind cluster and passes:
--- PASS: TestSeedDataDirLossDoesNotRebootstrap (246.70s), in a suite
finishing in 1018s against a 45m budget. The kamaji-datastore job runs the
whole test/e2e package unfiltered despite its name, so the new test is
covered there.

The wipe helper's verification step has since been tightened: the ephemeral
container's exit code now reflects whether the data dir is actually empty
afterwards. Previously the final ls -A always exited 0, so a partial wipe
would have been reported as success and surfaced much later as a seed that
simply starts fine. The check also fails closed when the data dir cannot be
read at all, which an emptiness test alone treats as "empty".